buffer: avoid 32-bit truncation in copy beyond 2 GiB#62942
Conversation
`Buffer.prototype.copy` and `copyArrayBuffer` use Uint32Value() to read offset and length arguments in C++. Values larger than 2**32 are silently truncated, so a 4 GiB+ copy quietly drops bytes. Switch to IntegerValue() and widen internal types to size_t so the slow path handles the full kMaxLength range. The fast path keeps its uint32_t signature; values beyond UINT32_MAX naturally fall through to the slow path. Add a regression test gated on NODE_TEST_LARGE_BUFFER (matches test-buffer-tostring-4gb.js gating) so default CI does not need to allocate >4 GiB. Fixes: nodejs#55422 Signed-off-by: Maruthan G <maruthang4@gmail.com>
nodejs-github-bot
added
buffer
rafaelmfried
left a comment
rafaelmfried
left a comment
There was a problem hiding this comment.
Thanks for taking #55422 on — it's been open a long time.
One thing that may save a review round-trip: this same fix was reviewed before, in #62500, where @Renegade334 left three concrete asks. This PR diverges from all three:
1. FastCopy types. In #62500, @Renegade334 asked to change the FastCopy argument and return types from uint32_t to uint64_t. This PR leaves FastCopy at uint32_t. I built Node and probed this: with the fast path warm (confirmed — FastCopy was being hit during warmup), a copy() whose source offset exceeds 2**32 was routed to SlowCopy, not FastCopy — so the uint32_t FastCopy isn't reached with a large offset in that path and data isn't corrupted there. Even so, widening it to uint64_t removes a latent truncation and keeps the types consistent with CopyImpl (now size_t).
2. Checked operations. @Renegade334 asked to avoid .ToChecked() and use the .To(&var) pattern with an explicit early return. This PR uses IntegerValue(...).ToChecked() + CHECK_GE.
3. Test cost. The test allocates 4+ GiB buffers in test/parallel/, gated behind NODE_TEST_LARGE_BUFFER — so it never actually runs in CI as written. #62157 suggested test/pummel/ for large-buffer tests; @Renegade334 suggested keeping the allocation minimal (one large buffer, copy a tiny slice).
The core change — CopyImpl / SlowCopy / CopyArrayBuffer widened to 64-bit — looks correct to me. Aligning with the #62500 review would probably help this land.
3 participants
Buffer.prototype.copyandcopyArrayBufferuse Uint32Value() toread offset and length arguments in C++. Values larger than 2**32
are silently truncated, so a 4 GiB+ copy quietly drops bytes.
Switch to IntegerValue() and widen internal types to size_t so the
slow path handles the full kMaxLength range. The fast path keeps
its uint32_t signature; values beyond UINT32_MAX naturally fall
through to the slow path.
Add a regression test gated on NODE_TEST_LARGE_BUFFER (matches
test-buffer-tostring-4gb.js gating) so default CI does not need
to allocate >4 GiB.
Note: I was unable to run a full local build on Windows (vcbuild.bat fails due to missing NASM and inaccessible WindowsApps Python). The patch mirrors the diagnosis posted by the issue reporter.
cpplintis clean. Looking forward to CI verification.Fixes: #55422